WIP feat: Index docs in a worker #1092
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gmaclennan
changed the title
|
Why was it necessary to migrate the coresTable for this? |
gmaclennan
commented
Sep 24, 2025
There was a problem hiding this comment.
This looks ok to me (I can't approve my own PR). I think it's important to have tests to ensure that the migration works (e.g. persisted cores are not forgotten) because this could be hard to catch in manual QA (it would require force-quitting the app at a specific point, and ensuring the other device is not on the same network when re-starting).
I'd be ok with merging this and doing the tests next, but soon, before QA for the next RC is done, so that we don't release this without tests for this important step.
Are the existing migration tests (once adapted to use workers) sufficient to check this? Or is there more we need to add. |
|
Not sure, I have a feeling that they are not. Easiest way to check is to comment out the migration code and see if any test fails. |
|
This would only be noticeable in this sequence:
You would only notice that cores are missing (eg they haven't been persisted) if device B reindexes, because the data will be in the indexes for B. Alternatively, if B syncs with a new device C after migration, are A's cores shared (eg does C get A's data from B) |
|
Cool yeah I'll see how I can simulate that. I'm also gonna enable workers for all tests first to see if that breaks anything. Seems our sync tests are having trouble already |
|
The sync test is flakey so there might be a race condition in there regarding indexing. Gotta figure that out before merge |
|
I feel that the failing test is the thing that needs to change. If you look at my annotated logs, the Invitee is rightfully refusing to replicate its config cores. |
RangerMauve
reviewed
Oct 8, 2025
|
|
||
| t.after(() => manager.stopLocalPeerDiscoveryServer()) | ||
| t.after(() => | ||
| manager.stopLocalPeerDiscoveryServer({ force: true, timeout: 0 }) |
There was a problem hiding this comment.
Likely this could be reverted once we figure out why this test was having trouble
This was referenced Oct 8, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Initial experimental implementation to fix #132